Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#21157][DocDb]: Fix load_balancer_max_over_replicated_tablets description to reflect per-table status #24406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michaelzheng67
Copy link

Currently, the flag '--load_balancer_max_over_replicated_tablets' does not accurately reflect the fact that the setting is meant to be applicable per table.

This commit looks to update the description of the flag to explicitly state that this settings is on a per-table basis (note: customers may already have flag overrides so naively changing flag name outright is not ideal)

Test Plan:
./yb_release works

Reviewers:

Reviewed By:

Subscribers:

Tags:

Differential Revision:

@CLAassistant
Copy link

CLAassistant commented Oct 11, 2024

CLA assistant check
All committers have signed the CLA.

…s description

Summary:
Currently, the flag '--load_balancer_max_over_replicated_tablets' does not accurately reflect
the fact that the setting is meant to be applicable per table.

This commit looks to update the description of the flag to explicitly state that this settings
is on a per-table basis (note: customers may already have flag overrides so naively changing
flag name outright is not ideal)

Test Plan:
./yb_release works

Reviewers:

Reviewed By:

Subscribers:

Tags:

Differential Revision:
Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for infallible-bardeen-164bc9 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit cedbd19
🔍 Latest deploy log https://app.netlify.com/sites/infallible-bardeen-164bc9/deploys/6709b7bf9ef8e000080cfe56
😎 Deploy Preview https://deploy-preview-24406--infallible-bardeen-164bc9.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for infallible-bardeen-164bc9 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit eeb6b72
🔍 Latest deploy log https://app.netlify.com/sites/infallible-bardeen-164bc9/deploys/6709b820d107100008a9f530
😎 Deploy Preview https://deploy-preview-24406--infallible-bardeen-164bc9.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@ddorian ddorian requested a review from ddhodge October 12, 2024 11:02
@ddhodge ddhodge added the area/documentation Documentation needed label Oct 15, 2024
@@ -522,7 +522,7 @@ Default: `2`

##### --load_balancer_max_over_replicated_tablets

Specifies the maximum number of running tablet replicas that are allowed to be over the configured replication factor.
Specifies the maximum number of running tablet replicas that are allowed to be over the configured replication factor per table.
Copy link
Contributor

@ddhodge ddhodge Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Specifies the maximum number of running tablet replicas that are allowed to be over the configured replication factor per table.
Specifies the maximum number of running tablet replicas per table that are allowed beyond the configured replication factor. For example, if RF=3 and this flag is set to 3, you can have maximum 6 tablet replicas per table.

@@ -68,7 +68,7 @@ DEFINE_RUNTIME_int32(load_balancer_max_concurrent_tablet_remote_bootstraps_per_t

DEFINE_RUNTIME_int32(load_balancer_max_over_replicated_tablets, 1,
"Maximum number of running tablet replicas that are allowed to be over the configured "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Maximum number of running tablet replicas that are allowed to be over the configured "
"Maximum number of running tablet replicas per table allowed in addition to the configured "

@@ -68,7 +68,7 @@ DEFINE_RUNTIME_int32(load_balancer_max_concurrent_tablet_remote_bootstraps_per_t

DEFINE_RUNTIME_int32(load_balancer_max_over_replicated_tablets, 1,
"Maximum number of running tablet replicas that are allowed to be over the configured "
"replication factor.");
"replication factor per table.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"replication factor per table.");
"replication factor.");

@@ -215,7 +215,7 @@ struct Options {
// time.
bool kAllowLimitOverReplicatedTablets = true;

// Max number of running tablet replicas that are over the configured limit.
// Max number of running tablet replicas that are over the configured limit per table.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Max number of running tablet replicas that are over the configured limit per table.
// Max number of running tablet replicas per table allowed over the configured RF.

@ddhodge
Copy link
Contributor

ddhodge commented Nov 4, 2024

@SrivastavaAnubhav Can you TAL

@SrivastavaAnubhav
Copy link
Contributor

I'm talking to Michael about this on community slack. The best fix would actually be to do the following: introduce a new flag called load_balancer_max_over_replicated_tablets_per_table and use that instead of load_balancer_max_over_replicated_tablets if set (otherwise default to load_balancer_max_over_replicated_tablets) in cluster_balance_util.h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Documentation needed
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants